Migrate UI Framework source into Kibana. #9192

cjcenizal · 2016-11-22T22:32:47Z

Addresses #8867

When we make changes to our UI by adding new components to the UI Framework, our dev process will look like this:

Run Kibana with npm start.
Run the UI Framework dev server with npm run uiFramework:start.
Check it out on localhost:8080.
Add a new component to the UI Framework.
Integrate it into Kibana and test it out in the Kibana UI.
Create examples in the UI Framework and check them out in the browser.
Submit PR like normal.

Changes

Add dependencies to package.json.
Add task for building UI Framework docs and serving locally.
Import UI Framework scss from autoload/styles.js instead of importing the CSS in base.less.

FAQ

I see React and Redux are now dependencies. Are we using them in Kibana?

No, they're only used to build the UI Framework documentation site. They make it really easy to add examples of new components. If anybody thinks we should use a different set of tools, suggestions and PRs are welcome!

Why are we using SCSS? We're already using LESS.

SCSS has better mixins, value interpolation, control directives, and other features. We can migrate all of our LESS to SCSS eventually, so we'll only be using one preprocessor.

- Add dependencies to package.json. - Add task for building UI Framework docs and serving locally. - Import UI Framework scss from autoload/styles.js instead of importing the CSS in base.less.

w33ble

quick comment and a question

w33ble · 2016-11-22T22:45:27Z

package.json

@@ -62,7 +62,8 @@
    "makelogs": "makelogs",
    "mocha": "mocha",
    "mocha:debug": "mocha --debug-brk",
-    "sterilize": "grunt sterilize"
+    "sterilize": "grunt sterilize",
+    "uiFramework:start": "./node_modules/.bin/webpack-dev-server --config src/ui_framework/webpack.config.js --hot --inline --content-base src/ui_framework/public/"


You don't need ./node_modules/.bin/when running npm scripts, the node_modules/.bin is included in the path by default in that case.

Thanks man!

w33ble · 2016-11-22T22:47:41Z

package.json

    "npm": "3.10.8",
+    "numeral": "1.5.3",


I think numeral has caused us some issues in the past, though I suspect your use is probably fine. Curious what you're using it for.

Actually, I don't think it's actually being used anywhere. Thanks for catching this, I'll remove it.

spalger · 2016-12-05T20:52:00Z

What do you think about unifying source related to the ui framework's doc site in src/ui_framework/doc_site? public directories have additional meaning/baggage that doesn't/shouldn't apply here, and I was initially caught off-guard by the webpack.config.js file.

I'm thinking:

src/ui_framework
  components/
    local_nav/
      index.scss
    index.scss
  doc_site/
    src/
      actions/
      components/
      ...
      index.html
    build/
      // webpack output target
    webpack.config.js

cjcenizal · 2016-12-05T21:12:36Z

@spalger That's a really good idea. I'll implement your suggestion. Out of curiosity, what are the additional meanings/baggage you associate with the public dir and where/how did you form that understanding of it? (Trying to educate myself.)

w33ble · 2016-12-05T22:58:35Z

In the context of plugins, public is kind of magic, and it has special rules for import. You can see this in Kibana too, where we have src/ui/public/**, and you require things as ui/**, but it's really reaching into the public path. I'm guessing that's the baggage @spalger was referring to.

cjcenizal · 2016-12-05T23:02:34Z

@w33ble Ah OK yeah, that makes sense in the context of Kibana. I was wondering if there was a broader convention beyond Kibana I wasn't familiar with.

cjcenizal · 2016-12-05T23:03:53Z

@spalger Implemented your directory structure suggestion.

@w33ble Addressed your comments.

w33ble

quick dependency question

w33ble · 2016-12-05T23:37:07Z

package.json

@@ -91,6 +92,7 @@
    "babel": "5.8.38",
    "babel-core": "5.8.38",
    "babel-loader": "5.3.2",
+    "babel-polyfill": "6.9.1",


Should we be using some v5 of this plugin? Is it compatible with our current Babel 5 setup?

Great catch man, looks like this isn't needed at all since we're using 5, not 6.

spalger

This seems really close but I have a few quibbles. The major one that I didn't bring up in inline comments, because it's pretty wide-spread, is the use of default exports. It's a pretty new rules that we have choosen to follow, but I think it would be a good idea to update this code to that standard before merging it.

There's at least one typo it would have prevented :) (creatExample)

spalger · 2016-12-05T21:01:29Z

src/optimize/base_optimizer.js

@@ -108,6 +108,10 @@ class BaseOptimizer {
              `css${mapQ}!autoprefixer${mapQPre}{ "browsers": ["last 2 versions","> 5%"] }!less${mapQPre}dumpLineNumbers=comments`
            )
          },
+          {
+            test: /\.scss$/,
+            loaders: ['style', 'css', 'sass'],


This probably should be using the ExtractTextPlugin so the CSS is included synchronously, before the JS (which loads asynchronously):

{ test: /\.scss$/, loader: ExtractTextPlugin.extract( 'style', `css${mapQ}!autoprefixer${mapQPre}{ "browsers": ["last 2 versions","> 5%"] }!sass${mapQPre}` ) }

spalger · 2016-12-05T21:05:34Z

src/ui_framework/README.md

+
+In short: we've outgrown it! Third-party CSS frameworks like Bootstrap and Foundation are designed
+for a general audience, so they offer things we don't need and _don't_ offer things we _do_ need.
+As a result, we've forced to override their styles until the original framework is no longer


we've been or we're?

spalger · 2016-12-10T00:02:56Z

src/ui_framework/doc_site/src/services/example/createExample.js

+  GuideExample,
+} from '../../components';
+
+export default function creatExample(examples) {


Misspelled createExample()

spalger · 2016-12-10T00:04:50Z

src/ui_framework/doc_site/src/components/guide_example/guide_example.jsx

+
+export default class GuideExample extends Component {
+
+  constructor(props, sections) {


Seeing a second argument to the constructor of a component perplexed me at first (especially since the second argument that components normally take is context).

I wonder if it would be less confusing if this class was defined inside the createExample function, and the closure was used to give that example component it's sections/examples.

(I also think createExample() could use a bit more color in it's name)

export function createExampleComponent(sections) { return class ExampleCompoent extends Component { // ... } }

I think you have a really interesting point, but I'd like to avoid too much re-architecting of how the docs site works within this PR -- I just want to focus on getting it integrated with Kibana for now, so we can unblock UI development. I'm totally open to refactoring it later though. :)

spalger · 2016-12-10T00:07:46Z

src/ui_framework/doc_site/src/actions/code_viewer/code_viewer_actions.js

+
+import ActionTypes from '../action_types';
+
+export default {


It's generally preferable to expose each value explicitly, rather than an object of named values.

ie.

export const openCodeViewer = slug => ({ type: ActionTypes.OPEN_CODE_VIEWER, slug, })

Same here; good point but let's do this later.

spalger · 2016-12-10T00:09:00Z

src/ui_framework/doc_site/src/components/guide_code_viewer/guide_code_viewer.jsx

+
+export default class GuideCodeViewer extends Component {
+
+  constructor(props) {


Unnecessary constructor, there are a few others

Same comment here... though I'm thinking once we add the linting rule for this, we can get rid of them all at once.

spalger · 2016-12-10T00:11:41Z

src/ui_framework/doc_site/src/components/index.js

@@ -0,0 +1,21 @@
+
+export * from './guide_code_viewer/guide_code_viewer.jsx';


This file seems to be exporting too much. Also, just like all other imports these imports should not have extensions.

Same... can we do this later?

spalger · 2016-12-10T00:12:37Z

src/ui_framework/doc_site/src/services/index.js

@@ -0,0 +1,12 @@
+
+export * from './example/createExample';


Another example of what looks like over-exporting

Same... can we do this later?

cjcenizal · 2016-12-10T00:35:06Z

@spalger Can you take another look? I addressed a few of your comments but I'd like to address the majority of them in subsequent PRs. I'd like to keep this PR focused on integrating the UI Framework as it already exists, so we can unblock UI dev (I'm continuing to work on the existing UI Framework and the longer this PR is left unmerged the more difficult it will be to port the changes over).

spalger · 2016-12-10T04:18:52Z

Sounds good to me @cjcenizal

w33ble · 2016-12-12T19:14:25Z

Code works as described. I run the commands, I get a running version of the framework. I'm not familiar with the vast majority of the stack here though, so I can't really give you considered input on that part.

I would like to say that I think it would be helpful to outline the conventions in the framework itself. Things like how you name classes with -- vs __, how to use scss variables correctly, how to organize files... basically all the rules around how the UI Framework works. Definitely in the running site, but perhaps also outlined in the top level readme.md file.

tylersmalley

LGTM - as it's been discussed we're simply reviewing how this project is to be brought into Kibana and saving the code change for another day.

tylersmalley · 2016-12-13T00:40:54Z

src/ui_framework/README.md

+## Development
+
+* Start development server `npm run uiFramework:start`.
+* View docs on `http://localhost:8080/`.


8080 is a common, and the default port for webpack dev server. We will probably want to change this in the future to avoid collisions.

tylersmalley · 2016-12-13T00:43:34Z

src/ui_framework/README.md

+
+## Examples of other in-house UI frameworks
+
+* [Ubiquiti CSS Framework](http://ubnt-css.herokuapp.com/#/app/popover)


Didn't you make this? 😄

Backports PR #9192 **Commit 1:** Migrate UI Framework source into Kibana. - Add dependencies to package.json. - Add task for building UI Framework docs and serving locally. - Import UI Framework scss from autoload/styles.js instead of importing the CSS in base.less. * Original sha: de3f34f * Authored by CJ Cenizal <[email protected]> on 2016-11-22T22:27:11Z **Commit 2:** Refactor UI Framework directory structure. * Original sha: 4e54844 * Authored by CJ Cenizal <[email protected]> on 2016-12-05T23:03:01Z **Commit 3:** Remove babel-polyfill. * Original sha: 43854bc * Authored by CJ Cenizal <[email protected]> on 2016-12-05T23:47:42Z **Commit 4:** Fix typos and include SCSS synchronously. * Original sha: fe5bab1 * Authored by CJ Cenizal <[email protected]> on 2016-12-10T00:33:17Z **Commit 5:** Update with latest changes to UI Framework. * Original sha: a44140a * Authored by CJ Cenizal <[email protected]> on 2016-12-13T01:47:09Z

Migrate UI Framework source into Kibana.

de3f34f

- Add dependencies to package.json. - Add task for building UI Framework docs and serving locally. - Import UI Framework scss from autoload/styles.js instead of importing the CSS in base.less.

cjcenizal force-pushed the 8867/feature/absorb-ui-framework branch from 40a96e5 to de3f34f Compare November 22, 2016 22:34

spalger self-assigned this Nov 22, 2016

w33ble reviewed Nov 22, 2016

View reviewed changes

cjcenizal added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. release_note:enhancement v5.2.0 v6.0.0 labels Nov 22, 2016

cjcenizal mentioned this pull request Dec 5, 2016

Introduce CSS preprocessor linting to warn against use of inline-block #9374

Closed

tylersmalley self-assigned this Dec 5, 2016

Refactor UI Framework directory structure.

4e54844

w33ble reviewed Dec 5, 2016

View reviewed changes

Remove babel-polyfill.

43854bc

spalger self-requested a review December 9, 2016 23:45

spalger suggested changes Dec 10, 2016

View reviewed changes

Fix typos and include SCSS synchronously.

fe5bab1

spalger approved these changes Dec 10, 2016

View reviewed changes

tylersmalley approved these changes Dec 13, 2016

View reviewed changes

Update with latest changes to UI Framework.

a44140a

cjcenizal merged commit 1e315d9 into elastic:master Dec 13, 2016

cjcenizal deleted the 8867/feature/absorb-ui-framework branch December 13, 2016 01:54

elastic-jasper mentioned this pull request Dec 13, 2016

[5.x] Migrate UI Framework source into Kibana. #9456

Merged

cjcenizal mentioned this pull request Dec 13, 2016

Move kibana-ui-framework into kibana repo. #8867

Closed

cjcenizal mentioned this pull request Feb 10, 2017

Remove dependency on kibana-ui-framework repo. #10281

Merged

AMoo-Miki mentioned this pull request Aug 3, 2021

[RFC] Charts & EUI: Forking vs Merging-in vs Folding-in opensearch-project/OpenSearch-Dashboards#695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate UI Framework source into Kibana. #9192

Migrate UI Framework source into Kibana. #9192

cjcenizal commented Nov 22, 2016 •

edited

Loading

w33ble left a comment

w33ble Nov 22, 2016

cjcenizal Nov 22, 2016

w33ble Nov 22, 2016

cjcenizal Nov 22, 2016

spalger commented Dec 5, 2016 •

edited

Loading

cjcenizal commented Dec 5, 2016

w33ble commented Dec 5, 2016

cjcenizal commented Dec 5, 2016

cjcenizal commented Dec 5, 2016

w33ble left a comment

w33ble Dec 5, 2016

cjcenizal Dec 5, 2016

spalger left a comment

spalger Dec 5, 2016

spalger Dec 5, 2016

spalger Dec 10, 2016

spalger Dec 10, 2016 •

edited

Loading

cjcenizal Dec 10, 2016

spalger Dec 10, 2016

cjcenizal Dec 10, 2016

spalger Dec 10, 2016

cjcenizal Dec 10, 2016

spalger Dec 10, 2016 •

edited

Loading

cjcenizal Dec 10, 2016

spalger Dec 10, 2016

cjcenizal Dec 10, 2016

cjcenizal commented Dec 10, 2016 •

edited

Loading

spalger commented Dec 10, 2016

w33ble commented Dec 12, 2016 •

edited

Loading

tylersmalley left a comment

tylersmalley Dec 13, 2016

tylersmalley Dec 13, 2016

cjcenizal Dec 13, 2016


		export default class GuideExample extends Component {

		constructor(props, sections) {


		export default class GuideCodeViewer extends Component {

		constructor(props) {

		@@ -0,0 +1,21 @@

		export * from './guide_code_viewer/guide_code_viewer.jsx';


		## Examples of other in-house UI frameworks

		* [Ubiquiti CSS Framework](http://ubnt-css.herokuapp.com/#/app/popover)

Migrate UI Framework source into Kibana. #9192

Migrate UI Framework source into Kibana. #9192

Conversation

cjcenizal commented Nov 22, 2016 • edited Loading

Changes

FAQ

w33ble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Dec 5, 2016 • edited Loading

cjcenizal commented Dec 5, 2016

w33ble commented Dec 5, 2016

cjcenizal commented Dec 5, 2016

cjcenizal commented Dec 5, 2016

w33ble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Dec 10, 2016 • edited Loading

spalger commented Dec 10, 2016

w33ble commented Dec 12, 2016 • edited Loading

tylersmalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Nov 22, 2016 •

edited

Loading

spalger commented Dec 5, 2016 •

edited

Loading

spalger Dec 10, 2016 •

edited

Loading

spalger Dec 10, 2016 •

edited

Loading

cjcenizal commented Dec 10, 2016 •

edited

Loading

w33ble commented Dec 12, 2016 •

edited

Loading